Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KTOR-7812 Use default JDK for running tests on CI #4342

Merged
merged 10 commits into from
Nov 22, 2024
Merged

KTOR-7812 Use default JDK for running tests on CI #4342

merged 10 commits into from
Nov 22, 2024
+150 −253

Conversation

osipxd
Copy link
Member

@osipxd osipxd commented Sep 25, 2024 •
edited
Loading

Subsystem
Tests infrastructure

Motivation
KTOR-7812 Use default JDK for running tests on CI

Solution
Use the JDK from JAVA_HOME to run tests on CI.

It is better to review commit by commit.

@osipxd osipxd self-assigned this Sep 25, 2024
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 689782f to 462fd68 Compare September 25, 2024 07:59
@bjhham
Copy link
Contributor

bjhham commented Sep 26, 2024

There seems to be some problems in the TLS package - we got an out-dated ECDH curve java.security.ProviderException: Curve not supported: secp128r1 (1.3.132.0.28) and some rogue reflection java.lang.reflect.InaccessibleObjectException: Unable to make private static java.time.Instant java.time.Instant.create(long,int) accessible: module java.base does not "opens java.time" to unnamed module @5b8bc155

You also seem to have unlocked a consistent failure in testHeaderIsTooLong(), so that's good 😄

@osipxd
Copy link
Member Author

osipxd commented Sep 26, 2024 •
edited
Loading

Yes, finally these builds against different Java versions found something :)

@osipxd osipxd marked this pull request as draft September 26, 2024 15:23
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 462fd68 to 5749278 Compare September 30, 2024 10:34
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 5749278 to 8210bed Compare October 7, 2024 13:42
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch 6 times, most recently from 719000a to a623b53 Compare November 19, 2024 08:23
osipxd
osipxd commented Nov 19, 2024
View reviewed changes
...client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CookiesIntegrationTests.kt
@@ -186,7 +186,7 @@ class CookiesIntegrationTests : ClientLoader() {
}

@Test
fun testCookiesWithWrongValue() = clientTests(listOf("js", "Darwin", "DarwinLegacy", "WinHttp")) {
fun testCookiesWithWrongValue() = clientTests(listOf("Js", "Darwin", "DarwinLegacy", "WinHttp", "Java")) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openjdk/jdk16@1c47244: Java 16 adds validation for header values, so this test doesn't pass on Java engine anymore

@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 433072c to 6f9571d Compare November 19, 2024 15:01
@osipxd osipxd marked this pull request as ready for review November 19, 2024 15:02
@osipxd osipxd requested a review from bjhham November 19, 2024 15:03
@osipxd
Copy link
Member Author

osipxd commented Nov 19, 2024

@bjhham, finally I've fixed this PR. After some tries I didn't manage to fix testHeaderIsTooLong, so I disabled it and created a task to fix it later:

  • KTOR-7811 Flaky test: SustainabilityTestSuite.testHeaderIsTooLong

@osipxd osipxd changed the title KTOR-7482 Use default JDK for running tests on CI KTOR-7812 Use default JDK for running tests on CI Nov 19, 2024
@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 6f9571d to 2e4099b Compare November 21, 2024 16:37
@osipxd osipxd requested a review from e5l November 21, 2024 16:37
@bjhham
Copy link
Contributor

bjhham commented Nov 22, 2024

Looks like there's just one test being a nuisance here io.ktor.tests.server.tomcat.jakarta.TomcatHttpServerCommonTest.

Might need to ignore it idk.

@e5l
Copy link
Member

e5l commented Nov 22, 2024

testHeadRequest requires investigation

@e5l
Copy link
Member

e5l commented Nov 22, 2024

@osipxd, could you put @ignore on this test in your branch? I'm investigating it in the separate one

e5l
e5l approved these changes Nov 22, 2024
View reviewed changes
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider adding retry for failing tests on native

@osipxd osipxd force-pushed the osipxd/run-tests-on-target-jvm branch from 2e4099b to adc9358 Compare November 22, 2024 09:11
@osipxd osipxd merged commit a3bccae into main Nov 22, 2024
14 checks passed
@osipxd osipxd deleted the osipxd/run-tests-on-target-jvm branch November 22, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjhham bjhham bjhham approved these changes

@e5l e5l e5l approved these changes

Assignees

@osipxd osipxd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@osipxd @bjhham @e5l